-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Adds DHCP Range Setup to APMode #6731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
uint8_t CIDR = WiFiGenericClass::calculateSubnetCIDR(subnet); | ||
// netmask must have room for at least 12 IP addresses (AP + GW + 10 DHCP Leasing addresses) | ||
// netmask also must be limited to the last 8 bits of IPv4, otherwise this function won't work | ||
if (CIDR > 28 || CIDR < 24) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does the netmask have to be the last 8 bits? What is wrong with being 16 bits instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to make it work correctly for netmask with more than 8 bits, it would be better to invert the byte order when IPAddress is cast to uint32_t.
I'm not sure that changing it would not break the code in other places or for other users.
Given that this issue has been already there since the very beginning, I just added code to prevent it from happening, limiting the netmask to 8 bits.
Example:
1.2.3.255 goes to 0xFF030201 in the current IPAddress Class casting method.
Adding 1, means to add (1 << 24) and it goes to 0x00030201 => IP 1.2.3.0 (it is worst when adding 11 << 24 to create a DHCP range).
If it were inverted, it would go to 0x010203FF, instead.
So adding 1 would result in 0x01020400 => IP 1.2.4.0, respectively, adding 11 would result in 0x01020410 => IP 1.2.4.10 (a lot better way to make the increment!)
It can be achieved by forcing this byte order inversion/reversion in the WiFiGenericClass code... is it worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inverting the bytes also makes it way easier to compare (uint32_t) IPAddress
(greater, less than) in order to check if it belongs to an IP range or if it could break netmask limits and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But inverting it will make it otherwise incompatible with everything else. We need to have the byte order that computers and IDF use. I do not see a reason why not add a method/methods to internally flip the order and do the operations agains another IP :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll add the code for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@me-no-dev - I tested it with netmask 255.255.0.0 for these cases:
1- AP = 192.168.1.250 and DHCP Range = 192.168.1.251 to 192.168.2.4
2- AP = 192.168.1.1 and DHCP Range = 192.168.2.1 to 192.168.2.10
In both cases the IDF call tcpip_adapter_dhcps_option()
fails and returns error 0x5001.
It only works if AP IP address and DHCP range are in the same network with netmask 255.255.255.0 to 255.255.255.240
So, it seems that IDF only works with 8 bits subnet.
Therefore, this PR is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed by looking into the esp_netif_lwip.c code:
https://github.com/espressif/esp-idf/blob/master/components/esp_netif/lwip/esp_netif_lwip.c#L1857-L1862
it only uses the last 8 bits of the AP address, limiting netmask effect to also 8 bits.
/*config ip information must be in the same segment as the local ip*/
softap_ip >>= 8;
if ((start_ip >> 8 != softap_ip)
|| (end_ip >> 8 != softap_ip)) {
return ESP_ERR_ESP_NETIF_INVALID_PARAMS;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is ready for merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also found out an IDF limit for the size of DHCP range: DHCPS_MAX_LEASE, default is 0x64 = 100 addresses (lwip/include/apps/dhcpserver/dhcpserver.h)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commited changes that prepare the code for any netmask (for the future, when IDF supports it as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameter changes to softAPConfig() look fine.
Can set_esp_interface_ip() be declared as a protected function in WiFiGeneric.h instead of declaring in each class that calls it?
Since this adds functionality rather than a bugfix, should this be targeted for release 2.1.0?
I'll leave it up to the maintainers to review changes in WiFiGeneric.cpp.
Yes, nice! :-)
We are reserving breaking changes for the 2.1.0. It doesn't break anything, therefore it is targeted for the 2.0.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All necessary changes are done.
uint8_t CIDR = WiFiGenericClass::calculateSubnetCIDR(subnet); | ||
// netmask must have room for at least 12 IP addresses (AP + GW + 10 DHCP Leasing addresses) | ||
// netmask also must be limited to the last 8 bits of IPv4, otherwise this function won't work | ||
if (CIDR > 28 || CIDR < 24) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is ready for merging.
After checking this suggestion, I say it is not possible because
|
Description of Change
This PR adds an optional parameter to
WiFi.softAPConfig()
in order to set the initial address of the DHCP Leasing Range.It has no impact over sketches that do not set this parameter because it will work as the default previous way.
The PR also improves IP verification for errors with subnet, collision with DHCP range and setting IPs out of the subnet range.
Tests scenarios
Tested on ESP32, ESP32-S2, ESP32-C3 and ESP32-S3.
Tested with different configurations for IPs, including using different net masks.
Related links
Fixes #6271